Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix trash-bin propfind responses #2821

Merged
merged 2 commits into from
May 4, 2022
Merged

fix trash-bin propfind responses #2821

merged 2 commits into from
May 4, 2022

Conversation

C0rby
Copy link
Contributor

@C0rby C0rby commented May 3, 2022

Fixed the href of the root element in trash-bin propfind responses.
Fixes: owncloud/ocis#1846

@C0rby C0rby requested a review from a team as a code owner May 3, 2022 13:36
@C0rby C0rby self-assigned this May 3, 2022
@phil-davis phil-davis mentioned this pull request May 4, 2022
Closed
@phil-davis
Copy link
Contributor

@C0rby the root href is now correct. I see things like:

listFolder for collectionPath '' level 1 returned:
      │ array(3) {
      │   [0]=>
      │   array(5) {
      │     ["href"]=>
      │     string(32) "/remote.php/dav/trash-bin/Alice/"
      │     ["name"]=>
      │     NULL
      │     ["mtime"]=>
      │     NULL
      │     ["collection"]=>
      │     bool(true)
      │     ["original-location"]=>
      │     NULL
      │   }
      │   [1]=>
      │   array(5) {
      │     ["href"]=>
      │     string(69) "/remote.php/dav/trash-bin/Alice/05bbef6c-6291-4473-92b0-5d9625c5e368/"
      │     ["name"]=>
      │     string(6) "PARENT"
      │     ["mtime"]=>
      │     string(10) "1651660731"
      │     ["collection"]=>
      │     bool(true)
      │     ["original-location"]=>
      │     string(6) "PARENT"
      │   }
      │   [2]=>
      │   array(5) {
      │     ["href"]=>
      │     string(69) "/remote.php/dav/trash-bin/Alice/eedec557-ffc5-48bc-b855-2ec5a73a35e4/"
      │     ["name"]=>
      │     string(6) "FOLDER"
      │     ["mtime"]=>
      │     string(10) "1651660731"
      │     ["collection"]=>
      │     bool(true)
      │     ["original-location"]=>
      │     string(6) "FOLDER"
      │   }
      │ }

The root entry has href "/remote.php/dav/trash-bin/Alice/" - correct.

And folders in the trashbin root have href like "/remote.php/dav/trash-bin/Alice/05bbef6c-6291-4473-92b0-5d9625c5e368/" - looks good.

But when the test code them traverses down to PROPFIND the folder in the trashbin, to find its contents:

      │ TrashbinTestInfo: collection has href: '/remote.php/dav/trash-bin/Alice/05bbef6c-6291-4473-92b0-5d9625c5e368/'
      │ recursive call to listTrashbinFolderCollection: 'Alice' '/05bbef6c-6291-4473-92b0-5d9625c5e368' '1'
      │ listFolder for collectionPath '05bbef6c-6291-4473-92b0-5d9625c5e368' level 2 returned:
      │ array(3) {
      │   [0]=>
      │   array(5) {
      │     ["href"]=>
      │     string(32) "/remote.php/dav/trash-bin/Alice/"
      │     ["name"]=>
      │     NULL
      │     ["mtime"]=>
      │     NULL
      │     ["collection"]=>
      │     bool(true)
      │     ["original-location"]=>
      │     NULL
      │   }
      │   [1]=>
      │   array(5) {
      │     ["href"]=>
      │     string(75) "/remote.php/dav/trash-bin/Alice/05bbef6c-6291-4473-92b0-5d9625c5e368/CHILD/"
      │     ["name"]=>
      │     string(5) "CHILD"
      │     ["mtime"]=>
      │     string(10) "1651660731"
      │     ["collection"]=>
      │     bool(true)
      │     ["original-location"]=>
      │     string(12) "PARENT/CHILD"
      │   }
      │   [2]=>
      │   array(5) {
      │     ["href"]=>
      │     string(79) "/remote.php/dav/trash-bin/Alice/05bbef6c-6291-4473-92b0-5d9625c5e368/parent.txt"
      │     ["name"]=>
      │     string(10) "parent.txt"
      │     ["mtime"]=>
      │     string(10) "1651660731"
      │     ["collection"]=>
      │     bool(false)
      │     ["original-location"]=>
      │     string(17) "PARENT/parent.txt"
      │   }
      │ }

it correctly lists folder "PARENT/CHILD" and file "PARENT/parent.txt".

But it lists "/remote.php/dav/trash-bin/Alice/" - the top-level root of the trashbin.

On oC10, a PROPFIND somewhere down in the trashbin lists the element that the propfind request was done on, for example, I expected to see /remote.php/dav/trash-bin/Alice/05bbef6c-6291-4473-92b0-5d9625c5e368/

  1. Is that easily fixed?

  2. The items that are listed have href like "/remote.php/dav/trash-bin/Alice/05bbef6c-6291-4473-92b0-5d9625c5e368/CHILD/". I am a bit surprised to see "CHILD" at the end. On oC10 the href is a list of file-id paths to the resource in the trashbin, so I thought it would be something like:
    `"/remote.php/dav/trash-bin/Alice/05bbef6c-6291-4473-92b0-5d9625c5e368/magic-long-hex-string-here-also/"

I haven't tried doing a PROPFIND to "/remote.php/dav/trash-bin/Alice/05bbef6c-6291-4473-92b0-5d9625c5e368/CHILD/" yet - maybe it will work?

I can work-around these differences in the test code, but if it can be made consistent with oC10 then it will be even easier, and clients will be able to reliably recurse into the trashbin contents, just like with oC10.

@C0rby
Copy link
Contributor Author

C0rby commented May 4, 2022

On oC10, a PROPFIND somewhere down in the trashbin lists the element that the propfind request was done on, for example, I expected to see /remote.php/dav/trash-bin/Alice/05bbef6c-6291-4473-92b0-5d9625c5e368/

Ah, yeah this is still a bug here and I'll fix it. Shouldn't be too hard.

I haven't tried doing a PROPFIND to "/remote.php/dav/trash-bin/Alice/05bbef6c-6291-4473-92b0-5d9625c5e368/CHILD/" yet - maybe it will work?

This should work IIRC.

I can work-around these differences in the test code, but if it can be made consistent with oC10 then it will be even easier, and clients will be able to reliably recurse into the trashbin contents, just like with oC10.

No, it definitely should be consistent. I'll try to fix it. 👍

@sonarcloud
Copy link

sonarcloud bot commented May 4, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@C0rby C0rby merged commit 10b760f into cs3org:edge May 4, 2022
@C0rby C0rby deleted the fix-trashbin-propfind branch May 4, 2022 14:01
@phil-davis
Copy link
Contributor

Good stuff. My testing works. I will provide updated test code to revert the previous work-around:
PR owncloud/core#40050 should be good code.
And then I can bump the core commit id etc.

@phil-davis
Copy link
Contributor

@C0rby it would be good to fix this in reva master also, then the same test code will pass in the same way.

Can you make a PR?

This was referenced May 5, 2022
Merged
Merged
@phil-davis phil-davis mentioned this pull request May 12, 2022
Open
@kobergj kobergj mentioned this pull request Oct 14, 2022
Merged
@kiranparajuli589 kiranparajuli589 mentioned this pull request Oct 20, 2022
Merged
@kiranparajuli589 kiranparajuli589 mentioned this pull request Oct 28, 2022
Merged
@kobergj kobergj mentioned this pull request Dec 19, 2022
Merged
@kobergj kobergj mentioned this pull request May 22, 2023
Closed
@kobergj kobergj mentioned this pull request Mar 1, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kobergj kobergj kobergj approved these changes

@phil-davis phil-davis phil-davis approved these changes

@labkode labkode Awaiting requested review from labkode labkode is a code owner

@ishank011 ishank011 Awaiting requested review from ishank011

@glpatcern glpatcern Awaiting requested review from glpatcern glpatcern is a code owner

Assignees

@C0rby C0rby

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@C0rby @phil-davis @kobergj